Skip to content

Panthers--Maggie T#92

Open
mjtice0 wants to merge 7 commits intoAda-C18:mainfrom
mjtice0:main
Open

Panthers--Maggie T#92
mjtice0 wants to merge 7 commits intoAda-C18:mainfrom
mjtice0:main

Conversation

@mjtice0
Copy link
Copy Markdown

@mjtice0 mjtice0 commented Dec 14, 2022

No description provided.

Comment thread index.html
Comment on lines +15 to +40
<div class="card">
<h1>Weather for <span id="city"></span></h1>
<div>
<input id="cityInput" type="text" size="30" placeholder="Enter city" />
<button id="citySearch">Search</button>
<button id="cityReset">Reset</button>
</div>

<h4>Temperature is <span id="temperature"></span>º F</h4>
<div>
<button id="decreaseTemp">-</button>
<button id="increaseTemp">+</button>
</div>
</div>

<div class = "landscapeSky">
<h4>Landscape is <span id="landscape"></span></h4>

<h4>Sky is <span id="sky"></span></h4>
<div>
<select id="skyInput">
<option value="sunny">Sunny</option>
<option value="cloudy">Cloudy</option>
<option value="rainy">Rainy</option>
<option value="snowy">Snowy</option>
</select>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markup looks great!

Comment thread styles/index.css
Comment on lines +54 to +75
#skyInput {
margin: 0;
width: calc(100% - 200px);
font-size: 105%;
border: 1px solid purple;
background-color: antiquewhite;
color: black;
font-family: inherit;
padding: 1.5em 1em;
float: left;

}

#decreaseTemp{
font-size: 36px;
font-weight: 600;
}
#increaseTemp {
font-size: 28px;
font-weight: 600;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work with styling 👍🏾

Comment thread styles/index.css
Comment on lines +2 to +11
body {
display: flex;
justify-content: center;
align-items: center;
height: 100vh;
margin: 30px;
background-color: rgb(136, 55, 211);
font-size: 120%;
flex-direction: column;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest adding media queries to account for all types of screen sizes

Comment thread src/index.js
Comment on lines +2 to +6
const state = {
sky: 'sunny',
temperature: 0,
city: '',
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having a hard coded default sky state, how might you refactor this so the logic is more data driven? The sky should really be displayed based on the call to the api

Comment thread src/index.js
temperature: 0,
city: '',
};
const temperatureChangeStep = 1;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer += temp or -= temp here 😁

Comment thread src/index.js
setTemperature(state.temperature - temperatureChangeStep);
};

const kelvinToFarenheit = (k) => 1.8 * (k - 273) + 32;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏾

Comment thread src/index.js

//use the city to get lat/long
const fetchCityLatLong = (city) => {
console.log('fetchCityLatLong()', city);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to remove debugging log statements

Comment thread src/index.js

// use lat/lon coordinates to get temperature
const fetchLatLongTemperature = (latLong) => {
const { lat, lon } = latLong;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Comment thread src/index.js
Comment on lines +30 to +33
elCityReset.addEventListener('click', (event) => {
setCity('');
elCityInput.focus();
});
Copy link
Copy Markdown

@ameerrah9 ameerrah9 Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an issue with wave 06 (resetting the city name), the requirements specify that you must include a button that resets the city name & when a user clicks on this button, the city name will be set to a default name along with the temperature for that city. Currently your code is not displaying the city name when you reset the city.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you reset the city it should be updated to a selected city instead of an empty string like so:

cityNameInput.value = 'Seattle';

Comment thread src/index.js
Comment on lines +166 to +174
//initialization - render all state first
renderCity();
renderTemperature();
renderSky();

//run initial search and show a temperature
setCity('Denver');
searchCityWeather('Denver');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since your script file is included at the end of the HTML file, it's probably fine to invoke this directly. But prefer to register it to be called in response to the DOMContentLoaded event. In the similar live code we did, we did call our onLoaded function directly, since codesandbox got in the way of our code being able to see the DOMContentLoaded event.

Copy link
Copy Markdown

@ameerrah9 ameerrah9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this project Maggie! The code you wrote for wave 6 is really close, I've put in a suggested solution. The code in this project is really clean and easy to read. This project is yellow 🟡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants